Enable relaxing of UnionEncoder requireRecordFields #62

dharmaturtle · 2021-02-14T21:16:08Z

I only did Newtonsoft.Json so far because System.Text.Json is proving annoying. I'll elaborate further if you like what's here.

Ref: #61

bartelink · 2021-02-15T09:07:08Z

src/FsCodec/FsCodec.fsproj

@@ -21,6 +22,13 @@
 <PackageReference Include="FSharp.Core" Version="3.1.2.5" Condition=" '$(TargetFramework)' == 'net461' " />
 <PackageReference Include="FSharp.Core" Version="4.3.4" Condition=" '$(TargetFramework)' == 'netstandard2.0' " />
 <PackageReference Include="FSharp.UMX" Version="1.0.0" />
+ <PackageReference Include="TypeShape" Version="8.0.0" />


I'm slightly reticent to add this dependency at this level.
My thinking is that the FsCodec package to date has largely only supplied contracts, and does not force many dependencies.
The scenario I had in my head is if someone is using an external library to provide the Codecs (and/or you're using the Create overloads that take an encode and tryDecode pair).
I'm not sure this matters.

This also slightly overlaps with my stance on using InternalsVisible and making things fully DRY by stuffing things into the core. My preferred technique is to have a source file somewhere in the tree and then add it as a reference to multiple projects. This allows one to e.g. keep FsCodec at 2.0.0 while still allowing one to update FsCodec.SystemTextJson to 2.4.0 without necessitating the same change for other modules in a subsystem that still use FsCodec.NewtonsoftJson.

I think this pattern should be followable here?

bartelink · 2021-02-15T09:22:34Z

tests/FsCodec.NewtonsoftJson.Tests/VerbatimUtf8ConverterTests.fs

+ | UInt64 of uint64
+ //| IntPtr of IntPtr // unsupported
+ //| UIntPtr of UIntPtr // unsupported
+ | Char of char


not sure about byte, sbyte and char - stuff that naturally maps to JSON seems a reasonable baseline ?

(remember Cosmos, and also things like SqlStreamStore.Postgres will impose their own constraints)

Also, related to comments on checkIfSupported, maybe the rules as to what's roundtripable should live in the store?

A case in point is VerbatimUtf8Converter: it is used by Equinox.Cosmos, but Equinox.CosmosStore (the V3 master impl), uses a different local impl (which, when we take it from jet/equinox#197 will be based on System.Text.Json).

Moving the code and/or mechanisms to live with the store might be an approach to take?

A counterpoint: While EventStore is happy to store any byte[] blob, it also has an IsJson which allows one to write JS projections, and in V20+ it has a Content-Type.

This suggests that a reasonable codec library might be general in nature and allow one to define a profile you're targetting in the abstract, which a concrete store library can define.

Going down that road would also mesh will with the notion of a contract checker which can be used to compile time check a contract adheres to rules - i.e. we might want to be able to define a convention in an integration test library that says "For this Domain lib, event contracts must be rountrippable on Equinox.Cosmos >=V3 and Equinox.EventStore >=V2, and all enums must be represented as strings (which implies having to add the converter attributes on the type)"

In that world, perhaps an FsCodec.Contracts library might depend on TypeShape and implement all this rather than shoving everything into FsCodec ?

A case in point is VerbatimUtf8Converter: it is used by Equinox.Cosmos, but Equinox.CosmosStore (the V3 master impl), uses a different local impl (which, when we take it from jet/equinox#197 will be based on System.Text.Json).

Moving the code and/or mechanisms to live with the store might be an approach to take?

Hm, pendulum swing: but Propulsion.Kafka.Codec also uses it.

OK, I think that suggests that there are multiple axes of constraints:

store-imposed: Equinox.Cosmos only wants JSON

more arbitrary: Propulsion.Kafka is looking to roundtrip a representation of an event from a store over Kafka

self-imposed: using Equinox.EventStore but we want to have all bodies be valid JSON so we can do JS projections

style: our team is using F# and we have decided our rule is we use records no matter what for reasons discussed in Relaxing requireRecordFields = true #61

not all roundtrips may be implemented in all libraries - i.e. some thorny thing might work in FsCodec.SystemTextJson but not in FsCodec.NewtonsoftJson

=> The rule checker may have a common impl to a degree, but we share any shareable bits by coimpiling the common code into a specific impl vs putting it into FsCodec

bartelink · 2021-02-15T09:24:55Z

tests/FsCodec.NewtonsoftJson.Tests/VerbatimUtf8ConverterTests.fs

@@ -80,7 +96,12 @@ module VerbatimUtf8Tests = // not a module or CI will fail for net461
 let des = JsonConvert.DeserializeObject<Batch>(ser, defaultSettings)
 let loaded = FsCodec.Core.TimelineEvent.Create(-1L, des.e.[0].c, des.e.[0].d)
 let decoded = defaultEventCodec.TryDecode loaded |> Option.get
- x =! decoded
+ match x, decoded with
+ | U.Double x, U.Double d when Double.IsNaN x && Double.IsNaN d -> ()


You can preserve the symmetry of having a type check either way by doing something like
| U.Double x, U.Double d when Double.IsNaN x => test <@ Double.IsNaN d @> (or true =! Double.isNan d is you prefer)

bartelink · 2021-02-15T09:25:53Z

tests/FsCodec.NewtonsoftJson.Tests/VerbatimUtf8ConverterTests.fs

+ match x, decoded with
+ | U.Double x, U.Double d when Double.IsNaN x && Double.IsNaN d -> ()
+ | U.Single x, U.Single d when Single.IsNaN x && Single.IsNaN d -> ()
+ | U.Double x, U.Double d -> Assert.Equal( x, d, 10)


🤔 does unquote provide a way to express this?

bartelink · 2021-02-15T09:27:52Z

src/FsCodec/Internal.fs

+ | Shape.Single _
+ | Shape.Double _
+ | Shape.Char _ -> true
+ | _ -> false


hm, consider explicitly listing the cases rather than leaving this open. If Half appears in this union, whats our stance?

bartelink · 2021-02-15T09:28:31Z

src/FsCodec/Internal.fs

+ | _ -> false
+ shape.UnionCases
+ |> Array.tryFind (not << isAllowed)
+ |> function


Option.iter ?

bartelink · 2021-02-15T09:32:12Z

src/FsCodec.NewtonsoftJson/Codec.fs

+ |> unbox
+ elif returnType = typeof<bool> then
+ json
+ |> System.Text.Encoding.ASCII.GetString


While this may be technically correct, I think I'd stick to using UTF8 in the interests of not overfitting? Same for Guid case.

Also this might make things easier / more sensible for STJ, which is v UTF8 centric

bartelink · 2021-02-15T09:37:20Z

because System.Text.Json is proving annoying

I know that feeling (and many around the world do!) 😄 see #59

bartelink · 2021-02-15T10:06:54Z

Thank you for the work thus far - this is looking very promising. Re #61, I had not done much thinking through / imagining of how this might look overall as your implementation is making me do now.

I covered some of these tradeoffs more in the code review comments; there may be things I've yet to consider which will prove some of the following wrong/unworkable, but my thoughts are:

I had thought that in principle each store should implement it's own checker - you've shown it can live here, and I like it
I still think I don't want to stuff things into the FsCodec core module but I may ultimately be wrong - someone that wants to write their own mapping functions and/or use a different serializer, they get to define (and maintain) their own rules. Putting helper modules in there also gives a false impression that FsCodec is a massively complex library; the intent is really to define a very minimal set of contracts indeed at that level.
considering how a contract style checker might be organized can be a useful tool for thinking about where stuff should live; expanding below on that concept

There are multiple axes of constraints:

store-imposed: Equinox.Cosmos can only store JSON - i.e. embedded byte[] content needs to be base64 encoded
e.g. DynamoDb might impose slightly different constraints than Cosmos
more arbitrary: Propulsion.Kafka is looking to roundtrip a representation of an event from a store over Kafka, which in general is fine if its UTF8
self-imposed: if usin using Equinox.EventStore, any byte[] is fine but we want to have all bodies be valid JSON so we can do JS projections
style: our team is using F# and we have decided our rule is we use records no matter what for reasons discussed in Relaxing requireRecordFields = true #61
not all roundtrips may be implemented in all libraries - i.e. some thorny thing might work in FsCodec.SystemTextJson but not in FsCodec.NewtonsoftJson
We want to define sets of rules that our IUnionContract types should observe as described in feat: Contract types validation #50 (One could imagine building a Roslyn analyzer checking the rules on the fly too)

bartelink · 2021-02-15T10:27:38Z

src/FsCodec.NewtonsoftJson/Codec.fs

@@ -119,14 +135,16 @@ type Codec private () =
 /// Configuration to be used by the underlying <c>Newtonsoft.Json</c> Serializer when encoding/decoding. Defaults to same as <c>Settings.Create()</c>
 [<Optional; DefaultParameterValue(null)>] ?settings,
 /// Enables one to fail encoder generation if union contains nullary cases. Defaults to <c>false</c>, i.e. permitting them
- [<Optional; DefaultParameterValue(null)>] ?rejectNullaryCases)
+ [<Optional; DefaultParameterValue(null)>] ?rejectNullaryCases,
+ /// Enables unions to contain a Guid or most primitives. Defaults to <c>true</c>, i.e. preventing Guids and primitives


require does not Enables ....; Perhaps something like:

/// Controls whether bodies of union cases are permitted to use primitive types, /// which can be harder to evolve when compared to always wrapping in a record in order to force all fields to have names. /// Defaults to <c>true</c>, i.e. preventing Guids and primitives from being used directly

dharmaturtle changed the title ~~Added requireRecordFields option to Newtonsoft.Json~~ Added requireRecordFields Feb 14, 2021

dharmaturtle force-pushed the requireRecordFields branch 4 times, most recently from 45854e8 to a374251 Compare February 15, 2021 01:38

Added requireRecordFields option to Newtonsoft.Json

3097d9a

dharmaturtle force-pushed the requireRecordFields branch from a374251 to 3097d9a Compare February 15, 2021 01:48

bartelink reviewed Feb 15, 2021

View reviewed changes

src/FsCodec/Internal.fs

| _ -> false

shape.UnionCases

|> Array.tryFind (not << isAllowed)

|> function

Copy link

Collaborator

bartelink Feb 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Option.iter ?

bartelink reviewed Feb 15, 2021

View reviewed changes

bartelink changed the title ~~Added requireRecordFields~~ Enable relaxing of UnionEncoder requireRecordFields Feb 15, 2021

bartelink reviewed Feb 15, 2021

View reviewed changes

dharmaturtle closed this Feb 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable relaxing of UnionEncoder requireRecordFields #62

Enable relaxing of UnionEncoder requireRecordFields #62

dharmaturtle commented Feb 14, 2021 •

edited

Loading

bartelink Feb 15, 2021

bartelink Feb 15, 2021

bartelink Feb 15, 2021

bartelink Feb 15, 2021

bartelink Feb 15, 2021

bartelink Feb 15, 2021

bartelink Feb 15, 2021

bartelink Feb 15, 2021 •

edited

Loading

bartelink commented Feb 15, 2021

bartelink commented Feb 15, 2021

bartelink Feb 15, 2021 •

edited

Loading

Enable relaxing of UnionEncoder requireRecordFields #62

Enable relaxing of UnionEncoder requireRecordFields #62

Conversation

dharmaturtle commented Feb 14, 2021 • edited Loading

bartelink Feb 15, 2021

Choose a reason for hiding this comment

bartelink Feb 15, 2021

Choose a reason for hiding this comment

bartelink Feb 15, 2021

Choose a reason for hiding this comment

bartelink Feb 15, 2021

Choose a reason for hiding this comment

bartelink Feb 15, 2021

Choose a reason for hiding this comment

bartelink Feb 15, 2021

Choose a reason for hiding this comment

bartelink Feb 15, 2021

Choose a reason for hiding this comment

bartelink Feb 15, 2021 • edited Loading

Choose a reason for hiding this comment

bartelink commented Feb 15, 2021

bartelink commented Feb 15, 2021

bartelink Feb 15, 2021 • edited Loading

Choose a reason for hiding this comment

dharmaturtle commented Feb 14, 2021 •

edited

Loading

bartelink Feb 15, 2021 •

edited

Loading

bartelink Feb 15, 2021 •

edited

Loading